fix: handle TypeError exposed by simple history logic#5476
fix: handle TypeError exposed by simple history logic#5476matthewelwell merged 2 commits intomainfrom
Conversation
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎ 3 Skipped Deployments
|
Docker builds report
|
Uffizzi Ephemeral Environment Deploying☁️ https://app.uffizzi.com/github.com/Flagsmith/flagsmith/pull/5476 ⚙️ Updating now by workflow run 15184201650. What is Uffizzi? Learn more! |
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #5476 +/- ##
==========================================
- Coverage 97.65% 97.65% -0.01%
==========================================
Files 1235 1235
Lines 43334 43333 -1
==========================================
- Hits 42317 42316 -1
Misses 1017 1017 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
…variate feature states
Zaimwa9
left a comment
Zaimwa9
left a comment
There was a problem hiding this comment.
Approved with minor comment
|
|
||
| def get_change_details(self) -> typing.Optional[typing.List[ModelChange]]: # type: ignore[return] | ||
| if self.history_type == "~": # type: ignore[attr-defined] | ||
| def get_change_details(self) -> typing.List[ModelChange]: |
There was a problem hiding this comment.
Just in case we could add a test to ensure it's covered
def test_get_change_details_handles_exception_prev_record_is_null():
instance = HistoricalFeatureState(history_type="~")
instance.prev_record = None
assert instance.get_change_details() == []
Ideally we could log when it happens but I let you decide if we will really track it or covering the case is enough
There was a problem hiding this comment.
Unfortunately it's not that easy. prev_record is a property, not an attribute, so I think any test that we add would involve more mocking than is really worthwhile.
Regarding the logging, since we've only seen this on a handful of cases, and the use case for this data is pretty slim, I don't think it's worth investigating further, hence negating the need for any additional logging.
I'm going to go ahead and merge this, but let me know if you disagree with either of my points above.
2 participants
Thanks for submitting a PR! Please check the boxes below:
docs/if required so people know about the feature!Changes
Fixes this Sentry issue.
How did you test this code?
Existing tests cover the lack of regression, this change is purely to handle a scenario that is not possible to rerproduce but has been seen in production as per linked sentry issue.